[Mobile Payments] Handle cancellation from card readers #8369
Conversation
Previously (prior to 5233d70 ) we didn’t dismiss any alerts when cancel was tapped on the reader (or built in reader modal) because we ignored the error. This corrects the comments, warnings, and errors produced (in the StripeCardReaderService) now that we don’t ignore the error any more. It also stops showing an alert (with a try again button) when the error is recieved in the CollectOrderPaymentUseCase, and instead dismisses the payment alerts. Now, in the new flow, we handle cancelations from card readers and from buttons in the app equally: we dismiss the alerts and track the cancelation events.
If the user cancels the payment on the reader, it can help the merchant to specifically notify them that the payment has not been taken, and they still need to capture payment. We do not do this with the built-in reader because it’s clear when everything happens on the same device.
joshheald
added
type: enhancement
You can test the changes from this Pull Request by:
|
|
@jostnes @lmischner: optional review but since you both noticed this, I thought it's worth tagging you here :) @toupper: no rush, I've set it for 11.7 |
|
@joshheald thanks for the PR! it tests well and the code looks good to me, I just added a couple of suggestions. |
| promise(.failure(CardReaderServiceError.paymentCancellation(underlyingError: underlyingError))) | ||
| DDLogWarn("💳 Warning: collect payment cancelled \(error)") | ||
| /// If we've not used the cancellable in the app, the cancellation must have come from the reader | ||
| if self?.paymentCancellable != nil { |
There was a problem hiding this comment.
Not a blocker but, is there another way to ascertain that the cancellation came from the reader? Tying it to whether the paymentCancellable is nil or not seems frail to me, we could change our implementation (e.g making paymentCancellable nil in another way) and it would break this logic.
There was a problem hiding this comment.
I agree, but there's no way that Stripe mention or that I was able to see. I'll check again but this has implications for the other comment on this PR (if there's no way to find out from Stripe, we won't be confident that it was cancelled on device when we get the error from Stripe)
|
|
||
| /// A command was cancelled on the reader. | ||
| /// Note that this is not produced by Stripe, we have to infer it from commandCancelled. | ||
| case commandCancelledOnReader |
There was a problem hiding this comment.
Since we already have a case commandCancelled, I would collapse these two checks, or at least add further information to the other case. Otherwise, it might be confusing: If commandCancelledOnReader is true, commandCancelled would be false, which is not a valid statement; the command was indeed canceled.
case commandCancelledOnDevice
case commandCancelledOnReader
or
enum CommandCancellationContext {
case device
case reader
}
...
case commandCancelled(context: CommandCancellationContext)
There was a problem hiding this comment.
Agreed, but I think that doing this depends on Stripe telling us the context when we get the error, as we need to convert from the Stripe commandCancelled before we have the detail of what triggered the cancellation.
While we could take the second suggestion and default to device, converting (as we do now) to reader when we have the context. There's no difference in code with these, but by saying device for reader cancellations too early, we're expressing confidence about something which we don't know for sure (commandCancelled could be either source)
Thanks for the review @toupper 😊 Unfortunately Stripe don't tell us what the cancellation source is, so we have to handle the unknown case. In bc89e2b I've added a cancellation source enum, which lets us sometimes update the cancellation to have the known type. In bee90e9 I've updated the service to add a flag which we set when a cancellation starts from the app. Then, if it's not set when we get a cancellation from Stripe during the collect step, we assume that it came from the reader. |
Closes: #8085
Description
In sTAP Away, we're adding support for Apple's built in card readers to take in-person payments. We continue to use Stripe's SDK to handle payments.
During this work, we are looking to share code where practical with the bluetooth reader flows, and fix issues in those flows which were less obvious with an external reader. One such issue is cancelation from card readers, including Apple's built in card reader.
On the built-in reader modal which Apple provide, there is a
xbutton to cancel the payment. On the WisePad 3 reader, there are customer-facing buttons, and tapping the redxbutton when collecting card details will cancel the payment.In both these cases, we should end the payment flow. For the WisePad 3 reader, we should inform the merchant that the payment was canceled, so they understand how the external device is impacting the payment flow.
Testing instructions
Using an iPhone XS or newer on iOS 16.0 or newer on a US store
Menu > Settings > Experimental featuresTap to Pay on iPhoneMenu > Payments > Collect paymentCardon the payment method screenTap to Pay on iPhoneand go through the Terms of Service Apple ID linking (if you've not done so before)xon Apple's screen to cancelcard_present_collect_payment_canceledtracks event is logged as expectedOn a CA store using a physical WisePad 3 reader – new flow
Menu > Settings > Experimental featuresTap to Pay on iPhoneMenu > Payments > Collect paymentCardon the payment method screenxbutton on the readercard_present_collect_payment_canceledtracks event is logged as expectedOn a CA store using a physical WisePad 3 reader – existing flow
Menu > Settings > Experimental featuresTap to Pay on iPhoneMenu > Payments > Collect paymentCardon the payment method screenxbutton on the readercard_present_collect_payment_canceledtracks event is logged as expectedScreenshots
WisePad 3 reader screen
Apple built in reader flow
cancel-payment-on-built-in-reader.mp4
RELEASE-NOTES.txtif necessary.